Skip to content

Conversation

@tobiasmelcher
Copy link
Contributor

Ensure problem annotation code minings are rendered below source lines to prevent visual overlap during vertical scrolling.

Fixes: #2512

redrawLines(lineIndex, getPartialBottomIndex() - lineIndex + 1, true);
}
}
redrawLines(lineIndex, initialTopIndex, initialBottomIndex, initialTopPixel, verticalIndentDiff);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, this block of code can be very expensive. I see it's now going to be executed more often than usual. Instead of fully removing the guard, isn't there a way to refine it?
However, I have not actually tried it on real big files. If you did so and are confident performance remains good, I'm fine with a merge, but code itself makes me unsure about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reduced it to "resetCache(lineIndex, 1);". Do you now have less concerns regarding performance?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resetCache per se is the operation that triggers a lot of time recomputing sizes and so on. So yes, adding this can be a concern for performance.
However, as mentioned, if you've happily tested on a big enough file, I would trust my concerns are vain and would be fine seeing this patch merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created following performance test to compare the runtime of the setLineVerticalndent method.

@Test
public void testPerformanceSetLineVerticalIndent() throws Exception{
	Shell shell = new Shell();
	shell.setLayout(new FillLayout());
	StyledText text = new StyledText(shell, SWT.V_SCROLL | SWT.H_SCROLL);
	shell.setSize(500,500);
	shell.setVisible(true);
	StringBuilder str = new StringBuilder();
	for (int i=0;i<10_000;i++) {
		str.append("Line ").append(i).append("\n");
	}
	int lineHeight = text.getLineHeight();
	text.setText(str.toString());
	while(Display.getDefault().readAndDispatch()) {
	}

	long start = System.currentTimeMillis();
	for (int i=0;i<10_000;i++) {
		text.setLineVerticalIndent(i, lineHeight*2);
	}
	long diff = System.currentTimeMillis()-start;
	System.out.println(diff);
}

On my machine, I see following numbers.
3609ms in the old code base
17318ms with the changes of this pull request where resetCache is called much more often. Factor 4.8 slower is quite high. But I would say that this is not a real world scenario. setLineVerticalIndent will never be called for 10.000 lines.

I don't really understand what method resetCache is doing and why it is not working properly in the scrolling scenario mentioned in the issue (#2512). Can you maybe give some more insights and give some hints which parts of the code might make the problem? I have the feeling that StyledTextRenderer#calculateIdle is not thread-safe.

I found a more efficient solution which will not has any performance impact by increasing the initialBottomIndex variable by value 2 in setLineVerticalIndent. But don't ask me for the reason, I cannot explain. I only see that the else if (lineIndex > initialBottomIndex) { branch is not complete from my point of view and the lines are then not correctly rendered together with the code minings.
image

How should we proceed here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Factor 4.8 slower is quite high.

I guess this factor is the same even for smaller files, or is it quadratic?

I don't really understand what method resetCache

resetCache() resets some info for the given lines (width/height and maybe even the position). Whenever it's invoked, it will cause cairo to recompute the particular line position) before next drawing completion.
I think maybe there is a way to make things smarter by shifting some line positions without clearing the cache.
However, I haven't touched that code for a while so I'm not sure I remember things correctly enough...

I found a more efficient solution which will not has any performance impact by increasing the initialBottomIndex variable by value 2 in setLineVerticalIndent

That's interesting. Maybe caching a few more lines ahead of time and anticipating the scrolling is a way to prevent for recomputing positions too often...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks a lot. Then I would propose to go for the +2 approach.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 15, 2025

Test Results

  111 files   -  7    111 suites   - 7   10m 29s ⏱️ - 1m 30s
4 375 tests  - 56  4 360 ✅  - 49  14 💤  - 3  1 ❌  - 4 
  242 runs   - 56    241 ✅  - 48   1 💤  - 3  0 ❌  - 5 

For more details on these failures, see this check.

Results for commit 3df8fde. ± Comparison against base commit 8c237c5.

This pull request removes 56 tests.
AllWin32Tests ImageWin32Tests ‑ testDisposeDrawnImageBeforeRequestingTargetForOtherZoom
AllWin32Tests ImageWin32Tests ‑ testDrawImageAtDifferentZooms(boolean)[1] true
AllWin32Tests ImageWin32Tests ‑ testDrawImageAtDifferentZooms(boolean)[2] false
AllWin32Tests ImageWin32Tests ‑ testImageDataForDifferentFractionalZoomsShouldBeDifferent
AllWin32Tests ImageWin32Tests ‑ testImageShouldHaveDimesionAsPerZoomLevel
AllWin32Tests ImageWin32Tests ‑ testRetrieveImageDataAtDifferentZooms(boolean)[1] true
AllWin32Tests ImageWin32Tests ‑ testRetrieveImageDataAtDifferentZooms(boolean)[2] false
AllWin32Tests ImageWin32Tests ‑ test_getImageData_fromCopiedImage
AllWin32Tests ImageWin32Tests ‑ test_getImageData_fromImageForImageDataFromImage
AllWin32Tests TestTreeColumn ‑ test_ColumnOrder
…

♻️ This comment has been updated with latest results.

@tobiasmelcher tobiasmelcher force-pushed the styled_text_set_line_vertical_indent branch 5 times, most recently from 3850b1b to 79283ab Compare September 18, 2025 16:07
@BeckerWdf
Copy link
Member

With the newest state of this PR I don't see the error happening any more.

Ensure problem annotation code minings are rendered below source lines
to prevent visual overlap during vertical scrolling.

Fixes: eclipse-platform#2512
@BeckerWdf BeckerWdf force-pushed the styled_text_set_line_vertical_indent branch from 79283ab to 3df8fde Compare September 19, 2025 08:36
@BeckerWdf
Copy link
Member

@mickaelistria: Can you pls. review again?

@BeckerWdf BeckerWdf added this to the 4.38 M1 milestone Sep 19, 2025
Copy link
Contributor

@mickaelistria mickaelistria left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a very good patch, as I think it actually has identified the root cause of the issue: as we're not fully sure of what's the last line when computing the lines to redraw, it makes much sense to iterate until the last one assuming a fixed/lowest line height to ensure we include all necessary lines.

@BeckerWdf
Copy link
Member

failing browser test (in org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser) look unrelated.

@mickaelistria mickaelistria merged commit 79e69bb into eclipse-platform:master Sep 19, 2025
16 of 17 checks passed
@mickaelistria
Copy link
Contributor

Thanks a lot @tobiasmelcher !

@jonahgraham
Copy link
Contributor

@tobias-melcher I think an earlier version of this commit was accidentally pushed to SWT in this branch https://github.com/eclipse-platform/eclipse.platform.swt/tree/styled_text_set_line_vertical_indent

If there is nothing left to do with it, can you delete this branch please?

@tobiasmelcher
Copy link
Contributor Author

@tobias-melcher I think an earlier version of this commit was accidentally pushed to SWT in this branch https://github.com/eclipse-platform/eclipse.platform.swt/tree/styled_text_set_line_vertical_indent

If there is nothing left to do with it, can you delete this branch please?

I've now deleted the branch. Apologies — I unintentionally pushed to the wrong remote.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rendering issue if "Show code minings for problem annotations" is enabled

5 participants